refactor(web-client): introduce ConfigBuilder for a better connection API#749
Conversation
Coverage Report 🤖 ⚙️Past: New: Diff: -0.01% [this comment will be updated automatically] |
f7d0b44 to
e5e21e2
Compare
456442d to
e90fe67
Compare
There was a problem hiding this comment.
General review:
- I like the new builder API.
- It’s well documented, thank you!
- But as for the extensions, it seems to me that we are basically back to something that is very close to the initial extension API proposal, but slightly worse because we can’t easily hide the string keys.
About my second point:
Here is what we have in this PR:
configBuilder
.withExtension('DisplayControl', true)
.withExtension('Pcb', pcb)
.withExtension('KdcProxyUrl', kdc_proxy_url);Here is what we would have using the Extension type defined in the initial proposal:
configBuilder
.withExtension(displayControlExtension(true))
.withExtension(pcbExtension(pcb))
.withExtension(kdcProxyUrlExtension(kdc_proxy_url));I’m open to dropping the Extension suffix from the helper functions above and aim for something like this:
configBuilder
.withExtension(displayControl(true))
.withExtension(preconnectionBlob(pcb))
.withExtension(kdcProxyUrl(kdc_proxy_url));In this second version, you can remove the Extension::init factory and associated interface.
The benefit is that it’s possible to remove the fragile string-based keys from the public API.
At this point, it’s also not necessary anymore to keep the serde-based dependencies that were previously introduced, and we can reduce the size of the resulting WASM module to what it used to be.
Requested follow ups:
- Update the
README.mdfile foriron-remote-desktopto document the changes. - Change the extension API to what was initially suggested as it seems to play better with the generic API exposed to the user.
Refactored
connectmethodIronRDP/web-client/iron-remote-desktop/src/services/PublicAPI.ts
Lines 16 to 27 in fe676ee
as discussed here - #722 (comment).
Follow-up to #722.